-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
typescript declarations emit #4375
Conversation
packages/react-admin/src/index.ts
Outdated
Module 'ra-core' has already exported a member named 'Notification'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308) | ||
Module 'ra-core' has already exported a member named 'Pagination'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308) | ||
*/ | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fzaninotto what's the correct symbols here should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ra-ui-material-ui should import those types from ra-core
There is many required props like
ReferenceInput
|
@user753 this is by no-mean full typescript coverage. |
@Bnaya I understand that. I think It could be fixed if all guessed props were optional. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please PR on next
, as this isn't a bug fix.
@@ -33,6 +33,9 @@ const useStyles = makeStyles( | |||
{ name: 'RaLoading' } | |||
); | |||
|
|||
/** | |||
* @type {React.FunctionComponent<{className: string; classes: object; loadingPrimary?: string; loadingSecondary?: string;}>} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer that you turn that component to TypeScript. I don't want to maintain a lib with 3 different syntax (JS, TS, and JS-with-types)
packages/react-admin/src/index.ts
Outdated
Module 'ra-core' has already exported a member named 'Notification'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308) | ||
Module 'ra-core' has already exported a member named 'Pagination'. Consider explicitly re-exporting to resolve the ambiguity.ts(2308) | ||
*/ | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe ra-ui-material-ui should import those types from ra-core
Also, before emitting the type definitions from ra-ui-material-ui, we must migrate the demo to TypeScript, so that we can make sure the types don't break existing apps. Would you like to tackle that task first? |
On the example you underline, if the source can be injected, in that case it should not be required in the types. |
e293c88
to
8bf3480
Compare
I'm willing to put effort to make that happen, but there are quite few things that are not clear to me |
@Bnaya Check this out #1617 (comment) :) |
@Bnaya What isn't clear and what do you want to talk about? |
I am really looking forward to this feature! It's really helpful! |
@fzaninotto since you closed this, I guess this is now superceded by #4505 ? |
Emit typescript declarations - utilize typescript 3.7 ability to emit types for js code.
Branched out from #3805
Minor code changes to make the types compilation pass